fix(workflows): reject a condition that has no {{ }} block - #4182

Both review points were correct — thanks. Fixed in c0c291c.

1. Unterminated / reversed delimiters slipped through

Confirmed against the interpolator rather than assumed. _interpolate_expressions() takes its raw_close == -1 branch and appends the tail verbatim, so nothing is substituted:

The helper now requires a complete block: it finds the first {{ and checks a }} follows it. {{ a }} {{ b }} and {{ inputs.text | default('}}') }} stay unflagged.

2. The correction was not valid YAML

Reproduced exactly as described — condition: "{{ inputs.name == "zzz" }}" raises yaml.parser.ParserError. A remediation the author cannot paste is not a remediation.

Added format_condition_correction() in expressions.py, used by all three validators as suggested. It picks quoting from the content — double by default, single when the expression holds a double quote, double with backslash escapes when it holds both — and drops a stray delimiter instead of nesting a second one, so {{ inputs.count > 100 corrects to "{{ inputs.count > 100 }}" rather than "{{ {{ ... }} }}".

Tests — the file goes from 40 to 70 cases. The new ones cover the incomplete forms, and run every correction through yaml.safe_load asserting it loads back to exactly {{ <expr> }} and that the corrected form is not itself re-flagged — the invariant that would have caught both of these.

uvx ruff@0.15.0 check src tests clean; pytest tests/unit 250 passed (the 2 symlink failures are a local Windows privilege limitation, green on CI).

@mnriem — both follow-up findings addressed in dc3c7de. Reproduced each one first rather than taking them on trust; both were real.

1. The }} check was not quote-aware

Confirmed exactly as described:

_interpolate_expressions finds no quote-aware close, falls to its raw-close branch, evaluates a truncated body and leaves residual text (False') — non-empty, so bool() makes it true.

Rather than restate the quote rules a third time in this module, I lifted the scan out of _interpolate_expressions into _find_block_close() and had the validator call it. The check and the substitution it is predicting now share one implementation and cannot drift — which is the same failure mode this PR is about. _interpolate_expressions keeps its exact previous behaviour; the extraction is behaviour-preserving.

2. Hand-rolled quoting missed control characters

Also reproduced — a condition carrying a newline did not round-trip:

Switched to json.dumps(..., ensure_ascii=False) as suggested — json was already imported, every JSON string is a valid YAML double-quoted scalar, and it removes the escape enumeration entirely. ensure_ascii=False keeps a non-ASCII operand readable instead of expanding it into \uXXXX; there is a test for that.

Tests 70 → 83. The quoted-delimiter condition joins the incomplete-block set; the round-trip set gains multiline, newline-with-quote, tab, carriage return and a non-ASCII operand. All four new cases fail on the previous commit (4 failed, 79 passed) and pass here (83 passed) — they are not vacuous.

Verification on this head: uvx ruff@0.15.0 check src tests — All checks passed. pytest tests/unit — 263 passed. pytest tests/workflows — 179 passed. The handful of local failures in both directories are the Windows symlink-privilege cases, identical on unpatched main; the CI matrix is the authority there and was green on the previous head.

Both suppressed findings in the latest Copilot pass are real. Fixed in aa9434a, and I reproduced each one against the committed code before touching it rather than taking them on trust.

1. Non-empty whitespace was excluded, and it should not have been

My docstring claimed a whitespace condition "coerces to False, which is a definite answer". That holds only for the empty string:

evaluate_condition("")      -> False
evaluate_condition("   ")   -> True
evaluate_condition("\t\n ") -> True

evaluate_condition strips only while testing the true/false keywords and then falls through to bool() on the raw string — and this repo pins that deliberately in test_condition_whitespace_only_string_stays_truthy. So condition: " " is precisely the silent always-true this helper exists to catch, and it was walking past the validator.

That existing test is why I fixed it at validation time rather than normalizing in the evaluator, which was Copilot's alternative: normalizing would contradict a behaviour the suite intentionally holds. The empty string stays excluded, because it really does coerce to False.

2. The correction removed only edge delimiters, so it could nest one

"prefix {{ inputs.ready"  ->  "{{ prefix {{ inputs.ready }}"

The suggestion carried an unclosed inner block, and because the outer block was complete, condition_is_never_evaluated accepted the corrected form — the validator waving through its own bad advice. Same shape with a trailing }}.

_strip_stray_delimiters now removes every delimiter and is quote-aware, for the reason the rest of this module is: inputs.x == '}}' holds a delimiter as data, and the blanket re.sub Copilot's first phrasing suggests would eat it and silently change what the condition compares against. I could not reuse _find_top_level — it counts { and } as bracket depth, so it never reports a {{ as a top-level token at all.

"prefix {{ inputs.ready"    -> "{{ prefix inputs.ready }}"
"inputs.ready }} suffix"    -> "{{ inputs.ready suffix }}"
"{{ inputs.x == '}}'"       -> "{{ inputs.x == '}}' }}"        <- delimiter kept as data
'{{ inputs.name == "a  b"'  -> '{{ inputs.name == "a  b" }}'   <- inner spacing kept

Whitespace collapses only where a delimiter was removed; inside a quoted operand nothing is touched.

One consequence I would rather name than leave for you to find. For a whitespace-only condition there is nothing to wrap, so the message ends Wrap the expression: "{{ }}". The diagnosis is right and the suggestion is empty. If you would rather the three validators omit the correction clause when there is no content to wrap, say so and I will thread that through — I left it alone because it means editing the message construction in if_then, while_loop and do_while, and I did not want to widen the diff while changes are requested.

Verification. tests/unit/test_condition_expression_block.py: 87 passed. The two fixtures that asserted whitespace was valid are corrected, and five cases added for interior delimiters, quoted delimiters and quoted spacing.

tests/test_workflows.py is 20 failed / 903 passed with this change, and 20 failed / 903 passed with it stashed — identical. All twenty are symlink tests that need Windows Developer Mode. Repo-wide the run is 168 failed / 6775 passed, and none of the 168 touches conditions or expressions; the same environment limit accounts for them.

@mnriem — this is on top of the two findings you raised, which went in as dc3c7de. As an outside contributor I cannot re-request review, so this comment is the only signal I have.

Third finding, and it holds too. Fixed in 3e28b32, reproduced before changing anything.

condition_is_never_evaluated was returning True for any {{ the quote-aware scan could not close — but _interpolate_expressions does not treat those alike, and its own comment says so. Two sub-cases, only one of which is "never evaluated":

{{ inputs.count > 100                  -> True             no raw '}}': emitted verbatim
}} inputs.count > 100 {{               -> True             same
{{ inputs.x == '}}'                    -> True             raw-close path, residual text
{{ inputs.missing | default('oops }}   -> raises ValueError

That last one made the message wrong on both halves: it is evaluated, and it does not end up true — it ends the run in _apply_filter.

Added condition_has_malformed_expression_block with its own branch in the three validators, because the two faults want opposite advice: one says you forgot the braces, the other says your delimiters or quotes do not balance. The predicates are mutually exclusive, pinned by a test over every fixture rather than left as an assumption.

The malformed branch deliberately offers no correction, which closes the loop on your earlier point about not auto-correcting malformed-block cases — it applies exactly here. The fault is unbalanced quoting, so the quote-aware stripper cannot tell operand from delimiter: for {{ inputs.missing | default('oops }} it produces

"{{ inputs.missing | default('oops }} }}"

which is not a fix. Naming the fault beats handing back something that looks authoritative and is not.

That also resolves the loose end I flagged last round: a blank correction now renders "{{ }}" instead of the double-spaced "{{ }}", so the whitespace case reads properly without needing to restructure the message.

Verification. 106 pass in tests/unit/test_condition_expression_block.py. Across tests/test_workflows.py + tests/unit: 22 failed / 1189 passed with this change, and 22 failed / 1170 passed with the diff stashed — the same 22, all Windows symlink cases needing Developer Mode, none touching conditions or expressions. The passed delta is exactly the tests added here.

@mnriem — that is three review rounds of findings now addressed (c0c291c, dc3c7de, aa9434a, 3e28b32). I still cannot re-request review as an outside contributor, so this comment is the only signal available to me.

@mnriem — the remaining Copilot finding is addressed in 183b633. It was correct.

The hole. Both validators stopped at the first {{, so a condition whose first block closes was accepted regardless of what followed. Measured against the code as it stood:

Both slipped through — the first is the silent-branching defect this PR exists to catch.

The fix. A shared _first_unclosable_block walks blocks exactly the way _interpolate_expressions does, continuing past each one that closes, and reports how the first unclosable block will fail: "evaluated" when a raw }} follows, "verbatim" when none does. Both validators read from it, so they cannot disagree with the substitution they are predicting — the same reason _find_block_close was already shared.

Two wording fixes fell out of scanning further, and I want to flag the second because I had it wrong:

1. The never-evaluated message said the condition "has no complete {{ }} block". Once an earlier complete block is possible that is false, so it now reads "is not a single complete {{ }} block".

2. condition_has_malformed_expression_block's docstring claimed the truncated body raises ValueError at run time. I checked both members of that class rather than assume:

   {{ inputs.missing | default('oops }}   -> ValueError
   {{ inputs.x == '}}'                    -> no raise; evaluates to "False'", truthy
   

   So the docstring over-claimed. The user-facing message never did — it says the interpolator "evaluates a truncated expression instead of the one written", which holds either way — but the docstring now says so too.

Verification on Python 3.11:

tests/unit/test_condition_expression_block.py      116 passed  (was 106)
tests/unit + tests/test_workflows.py              1199 passed  (was 1189)

The 22 failures in that second run are identical before and after this commit — all TestWorkflowCliAlignment symlink tests, which need elevation on Windows. I ran the suite on the stashed tree to confirm that rather than assume it.

Mutation-checked: restoring the stop-after-first-block behaviour fails exactly the 10 new parametrised cases and nothing else, so the new fixtures pin this specific defect rather than passing incidentally.

The other Copilot threads on this PR are marked outdated — they were addressed in dc3c7de, aa9434a and 3e28b32 (quote-aware scan shared with the evaluator, json.dumps quoting for the correction, whitespace-only special case, malformed-vs-never-evaluated split). Happy to walk through any of them if it is easier than reading back through the thread.

Thank you!

The suppressed finding on format_condition_correction was also correct — addressed in 2769ce3.

I measured what pasting each suggested correction actually does, rather than reasoning about it:

Both suggestions silently invert the condition. The blank core interpolates to the empty string; the open quote survives wrapping, so the raw-close fallback evaluates a truncated comparison whose result is the string "False", which evaluate_condition then reads as the false keyword. Either way the author pastes a "fix" and gets a different defect.

The fix. format_condition_remediation now builds the advice, and the three step validators call it instead of hand-assembling the sentence. It offers a correction only when wrapping would repair the input, and otherwise names the fault — the same call already made for condition_has_malformed_expression_block, which deliberately offers none.

'   '                    -> ... There is no expression here to wrap: use the literal
                             true or false, since an empty '{{ }}' block evaluates to
                             the empty string and would silently invert the condition.

{{ inputs.name == 'abc   -> ... Close the unbalanced quote first: wrapping it as
                             written leaves the quote open, so the raw-close fallback
                             evaluates a truncated comparison rather than the one
                             written, and its result can silently invert the condition.

inputs.count > 100       -> ... Wrap the expression: "{{ inputs.count > 100 }}".

_has_unbalanced_quote uses the same left-to-right scan as _find_block_close and _strip_stray_delimiters, so "inside a string" means one thing throughout the module.

One correction to my own work, since it happened in this commit. My first draft of the unbalanced-quote message said the wrapped form "stays always true". The new test failed and showed it returns False — evaluate_condition recognises the residual "False" as the keyword before bool() ever sees it. The message and docstring now say inverted, which is what the measurement shows.

Verification on Python 3.11:

tests/unit/test_condition_expression_block.py      133 passed  (was 116)
tests/unit + tests/test_workflows.py              1216 passed  (was 1199)

The 22 failures in the wider run are identical before and after — the TestWorkflowCliAlignment symlink tests, which need elevation on Windows. Confirmed by running the suite on the stashed tree, not assumed.

Mutation-checked: removing either gate fails exactly the 9 new parametrised cases and nothing else.

That clears every Copilot thread on this PR, including the suppressed one. @mnriem — ready for another look whenever suits.

Correction to my comment above: this merged at 13:24:16Z and I pushed 2769ce3 at 13:25:29Z, so the "ready for another look" was about a minute stale and that commit is not in the merge. My mistake — I checked mergeStateStatus and not state.

What did land in 145e5e688 is everything through 183b633ce, including the every-block scan. The remaining fix — not offering a correction that inverts the condition — is now #4230 against main, with the same verification and mutation check.

Thanks for the merge.